fix: harden complex parameter model loading - #2678
fix: harden complex parameter model loading#2678Rana Singh (ranadeepsingh) wants to merge 13 commits into
Conversation
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
|
Hey Rana Singh (@ranadeepsingh) 👋! We use semantic commit messages to streamline the release process. Examples of commit messages with semantic prefixes:
To test your commit locally, please follow our guild on building from source. |
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Pull request overview
This PR hardens SynapseML/SparkML model persistence and loading paths to fail closed against unsafe Java deserialization, unbounded metadata/object graphs, and path-traversal/link/glob issues—while preserving an explicit trusted-legacy compatibility switch for artifacts that cannot be safely constrained.
Changes:
- Introduces a session-scoped “trusted legacy artifact” gate (
spark.synapseml.legacy.allowUnsafeJavaDeserialization) and routes unsafe persistence (UDF closures, custom readers, BallTrees, DataFrames, etc.) behind explicit opt-in. - Adds hardened, budgeted model metadata/path handling (canonical containment, link resolution, bounded metadata enumeration and decoding budgets, recursive model context) for Pipeline/ComplexParams loads.
- Migrates
DataTypeParampersistence toward bounded JSON handling and adds/updates extensive regression coverage for security and compatibility.
Show a summary per file
| File | Description |
|---|---|
| reviews/serializer-deserialization/task-2513-attempt-1-review-1-gpt-5.6-sol.md | Adds a long-form review artifact documenting the hardening work and verification steps. |
| lightgbm/src/test/scala/com/microsoft/azure/synapse/ml/lightgbm/params/VerifyLightGBMBoosterParam.scala | Adds a regression ensuring LightGBM booster param loads under constrained legacy handling. |
| lightgbm/src/main/scala/com/microsoft/azure/synapse/ml/lightgbm/params/LightGBMBoosterParam.scala | Adds a narrow deserialization class policy for LightGBMBoosterParam. |
| core/src/test/scala/org/apache/spark/ml/VerifyArtifactPathResolver.scala | Adds tests for metadata listing guarantees and Java 8 linkage constraints. |
| core/src/test/scala/com/microsoft/azure/synapse/ml/stages/UDFTransformerSuite.scala | Adds trust-gating tests for persisted UDFs and session propagation through PipelineSerializer. |
| core/src/test/scala/com/microsoft/azure/synapse/ml/stages/LambdaSuite.scala | Enables trusted legacy load path for serialization fuzzing where needed. |
| core/src/test/scala/com/microsoft/azure/synapse/ml/recommendation/RankingTrainValidationSpec.scala | Enables trusted legacy load path for serialization fuzzing in ranking suites. |
| core/src/test/scala/com/microsoft/azure/synapse/ml/param/VerifyEvaluatorParam.scala | Adds explicit-trust tests for evaluator param persistence. |
| core/src/test/scala/com/microsoft/azure/synapse/ml/param/VerifyEstimatorArrayParam.scala | Adds coverage for stage-array persistence (safe writable vs trusted legacy cases). |
| core/src/test/scala/com/microsoft/azure/synapse/ml/param/VerifyDataTypeParam.scala | Adds coverage for JSON DataType persistence and legacy-stream gating/tripwires. |
| core/src/test/scala/com/microsoft/azure/synapse/ml/param/VerifyDataFrameParam.scala | Adds regression guarding against linked Parquet parts escaping artifact containment. |
| core/src/test/scala/com/microsoft/azure/synapse/ml/param/VerifyArrayParamMapParam.scala | Adds explicit-trust coverage for ArrayParamMapParam persistence. |
| core/src/test/scala/com/microsoft/azure/synapse/ml/nn/VerifySchemas.scala | Extends SafeObjectInputStream/BallTree coverage and validates fail-closed NN loading defaults. |
| core/src/test/scala/com/microsoft/azure/synapse/ml/io/split1/ParserSuite.scala | Enables trusted legacy load path for serialization fuzzing in parser suites where applicable. |
| core/src/test/scala/com/microsoft/azure/synapse/ml/core/utils/VerifySafeObjectInputStream.scala | Adds focused unit tests for SafeObjectInputStream resource and policy enforcement. |
| core/src/test/scala/com/microsoft/azure/synapse/ml/core/test/fuzzing/Fuzzing.scala | Adds a controlled trusted-legacy mode for serialization fuzzing and Pipeline/PipelineModel round trips. |
| core/src/test/scala/com/microsoft/azure/synapse/ml/core/serialize/VerifyModelLoadEnvironment.scala | Adds environment regressions around session scoping, trusted loading, and provider behaviors. |
| core/src/test/scala/com/microsoft/azure/synapse/ml/core/serialize/VerifyMetadataBudgets.scala | Adds regression ensuring aggregate decoded-metadata budgets are enforced. |
| core/src/test/scala/com/microsoft/azure/synapse/ml/core/serialize/ValidateComplexParamSerializer.scala | Significantly expands hardening regressions across metadata, paths, trust gates, and reader safety. |
| core/src/test/scala/com/microsoft/azure/synapse/ml/automl/VerifyFindBestModel.scala | Enables trusted legacy load path for serialization fuzzing in AutoML persistence tests. |
| core/src/main/scala/org/apache/spark/ml/StageReaderInspector.scala | Adds bytecode-based reader classification without class initialization. |
| core/src/main/scala/org/apache/spark/ml/Serializer.scala | Centralizes hardened serializer routing, trust scoping, Pipeline serializers, and safe read/write behavior. |
| core/src/main/scala/org/apache/spark/ml/ModelLoadContext.scala | Introduces shared load/write budgets (nodes, depth, metadata physical/decoded) across nested model loads. |
| core/src/main/scala/org/apache/spark/ml/DataTypeSerializer.scala | Implements bounded DataType JSON persistence with legacy-stream detection and UDT gating. |
| core/src/main/scala/org/apache/spark/ml/ComplexParamsSerializer.scala | Hardens ComplexParams read/write (session assignment, budgets, metadata accounting, and native pipeline boundaries). |
| core/src/main/scala/org/apache/spark/ml/ArtifactPathResolver.scala | Adds canonical containment, link resolution, bounded metadata enumeration/decoding, and safe directory validation helpers. |
| core/src/main/scala/com/microsoft/azure/synapse/ml/param/TransformerArrayParam.scala | Persists transformer arrays via hardened PipelineArraySerializer and validates load-time types. |
| core/src/main/scala/com/microsoft/azure/synapse/ml/param/EstimatorArrayParam.scala | Persists estimator arrays via hardened PipelineArraySerializer and validates load-time types. |
| core/src/main/scala/com/microsoft/azure/synapse/ml/param/DataTypeParam.scala | Adjusts DataTypeParam imports to align with new DataTypeSerializer path. |
| core/src/main/scala/com/microsoft/azure/synapse/ml/param/DataFrameParam.scala | Marks DataFrameParam as not supporting untrusted deserialization by default. |
| core/src/main/scala/com/microsoft/azure/synapse/ml/param/ByteArrayParam.scala | Adds an explicit deserialization class policy (primitive byte arrays). |
| core/src/main/scala/com/microsoft/azure/synapse/ml/param/BallTreeParam.scala | Marks BallTree params as requiring trusted loading (fail closed by default). |
| core/src/main/scala/com/microsoft/azure/synapse/ml/nn/BallTree.scala | Changes ConditionalBallTree default load to fail closed and adds explicit loadUnsafe. |
| core/src/main/scala/com/microsoft/azure/synapse/ml/core/utils/SafeObjectInputStream.scala | Expands SafeObjectInputStream with resource limits, filter composition, and class allowlisting model. |
| core/src/main/scala/com/microsoft/azure/synapse/ml/core/serialize/ComplexParam.scala | Adds per-param deserialization policy hooks and trust gating for unsafe complex params. |
Review details
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 34/35 changed files
- Comments generated: 2
- Review effort level: Lite
## Summary Clarify class-policy rejection messaging, remove machine-local paths from the review evidence, and isolate new UDF test imports so the security patch replays cleanly onto the spark4.1 compatibility branch. ## Prompting Intent Resolve every current-head pull-request comment and the release-branch compatibility failure without weakening the deserialization policy or changing branch-specific Spark 4.1 UDF behavior. ## Linked Sources - Pull request: microsoft#2678 - Class-policy feedback: microsoft#2678 (comment) - Review-artifact feedback: microsoft#2678 (comment) - Failed compatibility build: https://msdata.visualstudio.com/b9b2accc-2d1c-45b3-9d24-0eb5d78cc47f/_build/results?buildId=232896880 ## Rationale The class policy supports both exact names and package prefixes, so the error must describe the combined policy. Repo-relative evidence paths avoid leaking workstation details. Moving imports into the added tests keeps the functional master change intact while avoiding overlap with spark4.1's branch-only UDF helper object; a local three-way replay confirmed the complete patch applies cleanly to the current spark4.1 tip. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
## Summary Express Spark text framing from its explicit newline bytes and rename two persistence tests so their names identify the parameter type under test. ## Prompting Intent Resolve the current-head review comments precisely while preserving Spark 3.5's actual text-output framing semantics and keeping the security boundary tests understandable across master and spark4.1. ## Linked Sources - Pull request: microsoft#2678 - Framing review: microsoft#2678 (comment) - Evaluator test review: microsoft#2678 (comment) - ParamMap test review: microsoft#2678 (comment) - Spark 3.5 TextOptions: https://github.com/apache/spark/blob/v3.5.0/sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/text/TextOptions.scala ## Rationale Spark's text writer does not use the platform line separator by default; its TextOptions contract explicitly uses UTF-8 newline bytes. Computing the byte length from that literal documents the dependency without introducing incorrect Windows-specific accounting. Accurate test names improve failure diagnostics without changing coverage. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Review details
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
core/src/main/scala/org/apache/spark/ml/Serializer.scala:88
typeToSerializerroutes anyArray[_]whose element type is a subtype ofPipelineStagetoPipelineArraySerializerand then casts it toSerializer[T]. Since arrays are reified on the JVM,PipelineArraySerializerwill return anArray[PipelineStage], which cannot be safely treated as (for example)Array[Estimator[_]]/Array[Transformer]and can produceClassCastExceptionfor any caller that goes throughSerializer.typeToSerializer(outside the specialized Param overrides).
Consider returning a small adapter serializer for pipeline-stage arrays that converts to/from Array[PipelineStage] while preserving the requested runtime component type.
def typeToSerializer[T](
tpe: Type,
sparkSession: SparkSession,
classFilter: Option[DeserializationClassFilter]): Serializer[T] = {
(if (tpe <:< typeOf[PipelineStage]) new PipelineSerializer(sparkSession)
else if (isPipelineStageArray(tpe)) new PipelineArraySerializer(sparkSession)
core/src/main/scala/com/microsoft/azure/synapse/ml/core/utils/SafeObjectInputStream.scala:218
SafeObjectInputStream.ResourceFilteris a shared singleton instance ofDeserializationResourceFilter, butDeserializationResourceFilteris stateful (declaredArrayBytesaccumulates acrosscheckInputcalls). This makes the exposed filter instance easy to misuse and can make future tests order-dependent if they callResourceFilter.checkInputon multiple “allowed” inputs.
Prefer exposing a factory (e.g., def newResourceFilter(...)) that returns a fresh per-stream filter, and update VerifySafeObjectInputStream to construct a new instance for assertions.
private[utils] val ResourceFilter: ObjectInputFilter =
new DeserializationResourceFilter(defaultResourceLimits)
- Files reviewed: 35/35 changed files
- Comments generated: 0 new
- Review effort level: Lite
## Summary Preserve the requested JVM component type when generic serializers load PipelineStage arrays, provide fresh stateful resource filters per stream, and disambiguate Spark Transformer from Scala reflection for Spark 4.1 builds. ## Prompting Intent Resolve all suppressed current-head review findings and the spark4.1 compatibility compile failure without weakening the secure Pipeline serializer or its model-wide resource controls. ## Linked Sources - Pull request: microsoft#2678 - Suppressed current-head review body: microsoft#2678 - Failed spark4.1 build: https://msdata.visualstudio.com/b9b2accc-2d1c-45b3-9d24-0eb5d78cc47f/_build/results?buildId=232900732 ## Rationale JVM arrays are reified, so returning `Array[PipelineStage]` through a generic `Serializer[Array[Estimator[_]]]` cast is not type safe. A small adapter now validates elements and allocates the exact requested component array. Resource filters track aggregate array bytes and therefore must never be shared across streams. Fully qualifying Spark's Transformer avoids a Scala 2.13 reflection name collision while remaining source-compatible with the master baseline. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Resolved both suppressed findings from the
Master core compilation, main/test Scalastyle, |
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
## Summary Use overflow-safe multiplication and addition for declared array byte accounting, rejecting arithmetic overflow before it can weaken the deserialization resource budget. Add regressions for both per-array multiplication and aggregate addition overflow. ## Prompting Intent Resolve the current-head automated review finding on the model deserialization hardening PR while preserving fail-closed JEP 290 resource enforcement and cross-version compatibility. ## Linked Sources - Pull request: microsoft#2678 - Review comment: microsoft#2678 (comment) - Prior remediation: microsoft#2513 ## Rationale Exact arithmetic makes overflow an explicit rejection instead of allowing signed Long wraparound. This keeps the stateful aggregate budget monotonic and avoids saturation logic that could obscure malformed stream accounting. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
/azp run |
## Summary Fail closed when a ComplexParam object graph has no constrained deserialization policy, add per-type class filters for data-only parameters, and validate model metadata before loading payloads. Replace Java object streams with Spark Pipeline persistence for estimator and transformer arrays while retaining an explicit trusted-legacy compatibility path. ## Prompting Intent Investigate whether unrestricted Serializer.read behavior remained security-relevant after the earlier BallTree mitigation, close any reachable storage-to-compute code-execution paths, and preserve compatibility for trusted legacy artifacts without weakening secure defaults. ## Linked Sources - Prior partial mitigation: microsoft#2513 ## Rationale Per-type filters keep legitimate data-only object graphs loadable while preventing arbitrary classes and SerializedLambda callbacks from reaching readObject. Stage arrays use Spark's native persistence rather than another Java-serialization allowlist. Types that inherently capture executable closures fail closed and require a strongly named SparkSession opt-in so legacy compatibility is deliberate and auditable. Exact model-class and parameter-path checks prevent metadata pivots and payload redirection before deserialization begins. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
## Summary Complete the remaining model-persistence hardening with fail-closed Java deserialization, type-specific policies, bounded metadata and path handling, session-scoped trust, safe DataType JSON persistence, and explicit trusted compatibility for legacy native Spark Pipeline artifacts. ## Prompting Intent Investigate the remaining MSRC recommendation after the earlier public fix, determine whether unrestricted ComplexParam deserialization was still reachable, remediate the complete persistence boundary, preserve an explicit trusted-legacy path where safe migration is impractical, and provide extensive local regression coverage without exposing private incident details. ## Linked Sources - Prior public remediation: microsoft#2513 - Review evidence: reviews/serializer-deserialization/task-2513-attempt-1-review-1-gpt-5.6-sol.md ## Rationale Java deserialization callbacks execute before casts or parameter validation, and closure-bearing parameters cannot be secured with broad package allowlists. The implementation therefore defaults to non-executable formats or narrowly constrained object graphs, fails closed for arbitrary legacy payloads, and requires an explicit trusted scope for compatibility. Shared path, graph, stream, and metadata budgets address traversal, aliasing, compression, and resource-exhaustion risks across nested model graphs. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
## Summary Clarify class-policy rejection messaging, remove machine-local paths from the review evidence, and isolate new UDF test imports so the security patch replays cleanly onto the spark4.1 compatibility branch. ## Prompting Intent Resolve every current-head pull-request comment and the release-branch compatibility failure without weakening the deserialization policy or changing branch-specific Spark 4.1 UDF behavior. ## Linked Sources - Pull request: microsoft#2678 - Class-policy feedback: microsoft#2678 (comment) - Review-artifact feedback: microsoft#2678 (comment) - Failed compatibility build: https://msdata.visualstudio.com/b9b2accc-2d1c-45b3-9d24-0eb5d78cc47f/_build/results?buildId=232896880 ## Rationale The class policy supports both exact names and package prefixes, so the error must describe the combined policy. Repo-relative evidence paths avoid leaking workstation details. Moving imports into the added tests keeps the functional master change intact while avoiding overlap with spark4.1's branch-only UDF helper object; a local three-way replay confirmed the complete patch applies cleanly to the current spark4.1 tip. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
## Summary Express Spark text framing from its explicit newline bytes and rename two persistence tests so their names identify the parameter type under test. ## Prompting Intent Resolve the current-head review comments precisely while preserving Spark 3.5's actual text-output framing semantics and keeping the security boundary tests understandable across master and spark4.1. ## Linked Sources - Pull request: microsoft#2678 - Framing review: microsoft#2678 (comment) - Evaluator test review: microsoft#2678 (comment) - ParamMap test review: microsoft#2678 (comment) - Spark 3.5 TextOptions: https://github.com/apache/spark/blob/v3.5.0/sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/text/TextOptions.scala ## Rationale Spark's text writer does not use the platform line separator by default; its TextOptions contract explicitly uses UTF-8 newline bytes. Computing the byte length from that literal documents the dependency without introducing incorrect Windows-specific accounting. Accurate test names improve failure diagnostics without changing coverage. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
## Summary Preserve the requested JVM component type when generic serializers load PipelineStage arrays, provide fresh stateful resource filters per stream, and disambiguate Spark Transformer from Scala reflection for Spark 4.1 builds. ## Prompting Intent Resolve all suppressed current-head review findings and the spark4.1 compatibility compile failure without weakening the secure Pipeline serializer or its model-wide resource controls. ## Linked Sources - Pull request: microsoft#2678 - Suppressed current-head review body: microsoft#2678 - Failed spark4.1 build: https://msdata.visualstudio.com/b9b2accc-2d1c-45b3-9d24-0eb5d78cc47f/_build/results?buildId=232900732 ## Rationale JVM arrays are reified, so returning `Array[PipelineStage]` through a generic `Serializer[Array[Estimator[_]]]` cast is not type safe. A small adapter now validates elements and allocates the exact requested component array. Resource filters track aggregate array bytes and therefore must never be shared across streams. Fully qualifying Spark's Transformer avoids a Scala 2.13 reflection name collision while remaining source-compatible with the master baseline. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
## Summary Use overflow-safe multiplication and addition for declared array byte accounting, rejecting arithmetic overflow before it can weaken the deserialization resource budget. Add regressions for both per-array multiplication and aggregate addition overflow. ## Prompting Intent Resolve the current-head automated review finding on the model deserialization hardening PR while preserving fail-closed JEP 290 resource enforcement and cross-version compatibility. ## Linked Sources - Pull request: microsoft#2678 - Review comment: microsoft#2678 (comment) - Prior remediation: microsoft#2513 ## Rationale Exact arithmetic makes overflow an explicit rejection instead of allowing signed Long wraparound. This keeps the stateful aggregate budget monotonic and avoids saturation logic that could obscure malformed stream accounting. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
## Summary Add a fail-closed JEP 290 adapter that selects the Java 9+ java.io API or the Java 8 sun.misc backport at runtime without linking the published Scala sources to either interface. Keep per-stream class, graph, stream, and aggregate array limits active across supported Java versions. ## Prompting Intent Resolve the exact-head Azure Publish compilation failure under Temurin 8 without weakening the model deserialization hardening or the overflow fix requested by automated review, while retaining Spark 3.5 and spark4.1 compatibility. ## Linked Sources - Pull request: microsoft#2678 - Failed Azure build: https://msdata.visualstudio.com/b9b2accc-2d1c-45b3-9d24-0eb5d78cc47f/_build/results?buildId=232908468&view=logs&jobId=0ccccc7a-9630-5914-467b-15a9c61f0287 - Overflow review comment: microsoft#2678 (comment) - JEP 290: https://openjdk.org/jeps/290 ## Rationale Isolating runtime API differences behind a dynamic proxy preserves Java 8 source compatibility and per-stream JEP 290 enforcement on both API packages. This is safer than dropping resource filtering or changing the publication toolchain; runtimes without either supported filter API fail before reading an object graph. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
## Summary Scope the ArtifactPathResolver class-resource stream with the repository using helper so assertion or read failures cannot leak file or jar handles. ## Prompting Intent Resolve the current-head suppressed automated review finding without changing the Java 8 linkage assertion or production behavior, and preserve cross-version test compatibility. ## Linked Sources - Pull request: microsoft#2678 - Automated review head: microsoft@0036b77 ## Rationale Reusing StreamUtilities.using follows the existing test convention and guarantees closure on both success and failure without duplicating manual try/finally cleanup. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
## Summary Centralize trusted handling for test-generated model fixtures, bind Python model readers to the active Spark session, correct mismatched test readers, and route security coverage through executed CI shards. Remove the obsolete review artifact and support Scala 2.13 singleton serialization within the constrained LightGBM policy. ## Prompting Intent The engineer asked to finish the remaining MSRC deserialization remediation as a well-tested pull request, resolve review and CI failures, and reduce the number of changed files without weakening the security boundary. ## Linked Sources - MSRC incident and remaining-fix assessment: https://portal.microsofticm.com/imp/v5/incidents/details/31000000568481/msrc - Original partial remediation: microsoft#2513 - Follow-up pull request: microsoft#2678 ## Rationale Central fixture scoping replaces dozens of suite-specific trust flags while keeping production defaults fail-closed. Session binding is implemented once in JavaMMLReader rather than duplicated across generated wrappers. Native nested stages use the bounded PipelineSerializer, and Scala 2.13 compatibility permits only the exact serialization proxy while the referenced singleton class remains subject to the per-type allowlist. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
## Summary`nRestore Spark ML-compatible metadata part naming for bounded ComplexParams artifacts, align the ModelParam code-generation assertion with PipelineSerializer, and scope the SAR lazy-DataFrame round trip to an explicit trusted session load. ## Prompting Intent`nThe engineer asked to finish the MSRC deserialization remediation as a well-tested pull request, resolve all CI failures, and keep the follow-up narrowly focused instead of adding broad or suite-specific unsafe-deserialization exceptions. ## Linked Sources`n- MSRC incident and remaining-fix assessment: https://portal.microsofticm.com/imp/v5/incidents/details/31000000568481/msrc`n- Original partial remediation: https://github.com/microsoft/SynapseML/pull/2513`n- Follow-up pull request: microsoft#2678 ## Rationale`nWriting metadata directly as part-00000 preserves the bounded, session-backed serializer while retaining Spark ML and sparklyr on-disk compatibility; changing individual R suites would only hide the production contract break. The SAR test enables trust only around loading its locally created lazy DataFrame parameters and restores the previous session state, so production remains fail-closed. The remaining Scala change updates a stale assertion rather than altering runtime behavior. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
## Summary Route generated R complex-stage loading through the bounded PipelineSerializer and add a same-thread, session-bound trusted artifact scope for language bindings. Generated Python tests use the scope so MLflow's native PipelineModel persistence remains compatible without making configuration-only native loading trusted. ## Prompting Intent The engineer asked to complete the remaining MSRC insecure-deserialization mitigation, make PR microsoft#2678 merge-ready, reduce unnecessary file churn, run focused tests across supported Spark baselines, and resolve the deterministic Python and R CI failures without weakening the fail-closed production boundary. ## Linked Sources - IcM incident: https://portal.microsofticm.com/imp/v5/incidents/details/31000000568481/msrc - Prior partial fix: microsoft#2513 - Current pull request: microsoft#2678 ## Rationale Py4J and sparklyr cannot enter a Scala closure-based trust helper, while MLflow calls native PipelineModel persistence internally. A closeable token preserves the existing Spark-session, gateway-thread, and aggregate metadata-budget boundaries instead of allowing ambient configuration to authorize native Pipeline loading. Centralizing the R loader on PipelineStageWrappable removes duplicate unsafe ml_load paths and avoids adding more compatibility files. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
## Summary Route generated R fixture loading for nested pipeline stages through the bounded PipelineSerializer path while retaining each target branch's existing rLoadLine implementation. This removes the Spark 4.1 patch conflicts and drops PipelineStageParam.scala and TransformerParam.scala from the aggregate PR diff. ## Prompting Intent The engineer asked to make the MSRC deserialization remediation pull request merge-ready, run the relevant tests, resolve CI failures and review feedback, and reduce unnecessary file churn without weakening the fail-closed production boundary. ## Linked Sources - IcM incident: https://portal.microsofticm.com/imp/v5/incidents/details/31000000568481/msrc - Original partial remediation: microsoft#2513 - Remediation pull request: microsoft#2678 - Azure validation checks: https://github.com/microsoft/SynapseML/pull/2678/checks ## Rationale The Spark 4.1 branch intentionally uses a different sparklyr extraction expression, so changing rLoadLine on master created textual port conflicts despite equivalent behavior. Selecting the bounded loader at the sole R fixture-generation call site preserves branch-specific source and public APIs, keeps generated tests on PipelineSerializer with an explicit Spark session, and makes the complete patch apply cleanly to Spark 4.1. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
d929930 to
72a61f5
Compare
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
## Summary Close the caller-provided input stream when SafeObjectInputStream construction fails, preserving any close failure as a suppressed exception. Add a malformed-header regression test that verifies constructor-time failures cannot leak the source stream. ## Prompting Intent The engineer asked to finish the MSRC deserialization remediation as a well-reviewed, fully tested pull request and resolve all review comments without unnecessary file churn. ## Linked Sources - IcM incident: https://portal.microsofticm.com/imp/v5/incidents/details/31000000568481/msrc - Original partial remediation: microsoft#2513 - Remediation pull request: microsoft#2678 - Review finding: microsoft#2678 (comment) ## Rationale The existing using helper acquires ownership only after its argument is constructed, so it cannot close the original stream if ObjectInputStream header parsing or JEP 290 filter installation throws. A narrow NonFatal construction guard closes the source, retains the original failure, and records any close error as suppressed without changing successful read behavior. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Review details
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
core/src/main/python/synapse/ml/core/schema/Utils.py:141
ComplexParamsMixin.read()/JavaMMLReaderclaim to bind readers to the active Spark session, but the constructor always usesSparkSession.builder.getOrCreate(). This can ignore a thread-local active session (e.g.,spark.newSession()with different conf), undermining the session-scoped trust/config model described in the PR.
def __init__(self, clazz):
super(JavaMMLReader, self).__init__(clazz)
self.session(SparkSession.builder.getOrCreate())
core/src/main/scala/org/apache/spark/ml/ArtifactPathResolver.scala:429
resolveTreeInsiderecursively enumerates the entire directory tree viafs.listStatus(...)with no budgeting and without integrating withModelLoadContext. When used for DataFrame params (Parquet), this adds a full extra traversal and can be unbounded in both time and memory for large artifacts, which is at odds with the PR’s stated “bounded enumeration” goal. Consider iterating withlistStatusIteratorand counting each resolved entry viaModelLoadContext.current.foreach(_.enterPath(...))so model loads inherit the existing node limits and detect aliasing early.
def validateDirectory(directory: Path): Unit = {
val directoryKey = directory.toUri.normalize().toString
require(
visitedDirectories.add(directoryKey),
s"$description contains a filesystem-link cycle at $directory"
- Files reviewed: 45/45 changed files
- Comments generated: 0 new
- Review effort level: Lite
Related Issues/PRs
Follow-up to #2513 and the remaining MSRC recommendation in IcM 31000000568481.
What changes are proposed in this pull request?
This PR closes the remaining unrestricted Java deserialization path in
ComplexParamsmodel persistence. A crafted artifact could previously executedeserialization callbacks before the eventual type cast.
The patch:
type-specific class policies;
array, string, stream, metadata, path, and heap-aware limits;
possible;
and bounded remote enumeration before reader construction;
artifact load;
PipelineSerializerfor generated nested-stage tests;MLflow Pipeline persistence remains compatible without trusting ambient
configuration;
PipelineSerializerinsteadof native
ml_load; andsuite-specific opt-outs.
Compatibility contract
New data-only parameter artifacts load without unsafe Java deserialization.
Legacy artifacts containing arbitrary closures, BallTrees, DataFrames, native
readers, or native Spark Pipeline nesting must be explicitly treated as
trusted by setting
spark.synapseml.legacy.allowUnsafeJavaDeserialization=trueon the reader'sSpark session. Native Pipeline compatibility must additionally run inside
Serializer.withTrustedArtifactLoad; language bindings that cannot pass aScala closure use
Serializer.beginTrustedArtifactLoadand close the returnedscope on the same gateway thread. Untrusted nested models use SynapseML's
bounded
PipelineSerializer.Direct
ConditionalBallTree.loadfails closed. Trusted legacy callers can usethe explicit
loadUnsafeAPI.Scope
The effective diff contains 45 files: 20 connected production
security-boundary files and 25 focused test/harness files. It contains no review
artifacts, dependency changes, workflow changes, or generated
target/files.The final cross-language compatibility fix stays within files already required by
the remediation and adds no files.
How is this patch tested?
compilation and Scalastyle.
Pipeline/ComplexParams, generated-loader, UDF, recommendation, and
persistence suites.
the Spark 4.1 replay, including R codegen and the LightGBM policy test.
main/test Scalastyle on both baselines.
passed end-to-end through MLflow save, log, and load:
RankingAdapterModelSpec,RankingTrainValidationSplitModelSpec,RecommendationIndexerModelSpec, andSARModelSpec.learning (3), including trust cleanup assertions.
generated Python syntax compilation.
session-backed
PipelineSerializer/ml_call_constructorpath and contain nonested-model
ml_loadpath.a17cabb005.git diff --check.focused security/compatibility tests, and main/test Scalastyle.
Does this PR change any dependencies?
Does this PR add a new feature? If so, have you added samples on website?